-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added type checks for cpan authors #744
Added type checks for cpan authors #744
Conversation
self.vendor = metadata["author"].join(", ") unless metadata["author"].nil? | ||
# author is not always set or it may be a string instead of an array | ||
unless metadata["author"].nil? | ||
if metadata["author"].respond_to?(:to_str) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather have this check if it's a String, instead of if it responds to :to_str; the contract (if there even is such a thing) on what to_s
means vs to_str
is not really clear in the docs and certainly not clear to me as a reader :P
I'll change this post-merge.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I actually wasn't sure if I should check for the type or the method. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there's any strict rule to play here, Ruby's a very flexible language in this respect. My personal habit is to either check for type or check for a method. Oddly, I've never seen this 'to_ary' and 'to_str' methods used in 7 years of ruby programming. Weird! :P
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For my own learning -
Poking around String#to_str
has really poor docs "returns the receiver" as does Array#to_ary
which says "returns self".
Wondering what else has to_ary
I asked for Enumerator#to_ary
docs, and this method doesn't exist. Range#to_ary
also does not exist.
Based on this, I think to_str
and to_ary
are weird, unclear, and we should not use them.
I'll merge as-is and switch to using type checks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do believe that in most cases multiple ways of getting the object type will work.
As for this...response_to?
makes it (imho) more duck-type-like than just checking the type of object.
You dont really get advantages from respond_to?
in this case.
Nevertheless to_str
and to_ary
are no methods, they are symbols for the respond_to?
method.
Added type checks for cpan authors
Added type checks for cpan authors
Added type checks for cpan authors
Added type checks for cpan authors
Added type checks for cpan authors
CPAN 'authors' can be a list of strings, just a string or not set at all. This adds type checks to handle the different possibilities. Related to #712